Skip to content

Conversation

@burgesQ
Copy link
Contributor

@burgesQ burgesQ commented Jun 29, 2025

Description

Update the way cached makefile metadata are cached.\
Up until now, a single cache item (7s TTL) was generated whatever makefile was used.\

This new implementation allows generating a cache item per makefile path + content pair; allowing to get rid of the TTL.

One may enable the new strategy by setting the following prior to source fast-synthax-highlighting

typeset -gA FAST_HIGHLIGHT
FAST_HIGHLIGHT[chroma-make-cache-global]=1

Related Issue(s)

Some comments following https://github.com/zdharma-continuum/fast-syntax-highlighting/pull/81/files (dcee72b#commitcomment-161064002 & other)

Motivation and Context

Parsing Makefile targets is now expensive because of me, so let's attempt to run it as less as we needs

Usage examples

typeset -gA FAST_HIGHLIGHT
FAST_HIGHLIGHT[chroma-make-cache-global]=1

source ~/.zsh/fast-syntax-highlighting/fast-syntax-highlighting.plugin.zsh

How Has This Been Tested?

Locally 🤓

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation change
  • New feature (non-breaking change which adds functionality)

Checklist:

  • All new and existing tests passed.
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.

burgesQ referenced this pull request Jun 29, 2025
* fix(make): colorize targets from imported make-files

Old behavior use to parse the content of the invoked `Makefile` unit
to define targets.

Such parsing didn't take into account targets from unit imported by the
current (ex: targets defined in files imported such as `import
./targets/help.mk`).

New behavior pass the content of the Makefile unit post lexing (aka
`make -f Makefile -pn` instead of the plain unit.

* Update →chroma/-make.ch

---------

Co-authored-by: Philipp Schmitt <[email protected]>
@HearseDev
Copy link

There should be a regression to the old behavior until there is a solution that causes no delay, because this will cause a delay on each key presses on larger makefile units.

@burgesQ
Copy link
Contributor Author

burgesQ commented Jun 30, 2025

@HearseDev I'll introduce my enhancements with this MR (shared caching, one caching item per path). Probably @pschmitt or some other maintainer could revert in another dedicated MR #81 until then
?

@phush0
Copy link

phush0 commented Jul 3, 2025

I saw this one too, hope it will be merged fast

edit: deffinitly better with this PR, but far from perfect after you type make c there is delay than you can complete lean it is like that for every subcommand, if file is not changed I don't see reason to be parsed again and again

@burgesQ
Copy link
Contributor Author

burgesQ commented Jul 3, 2025

I saw this one too, hope it will be merged fast

edit: deffinitly better with this PR, but far from perfect after you type make c there is delay than you can complete lean it is like that for every subcommand, if file is not changed I don't see reason to be parsed again and again

Working locally on some version where units are re-parsed only when the content has changed. Should be published by the end of the day

@phush0
Copy link

phush0 commented Jul 3, 2025

I added FAST_HIGHLIGHT[chroma-make-cache-global]=0, because it will use old behaviour

@burgesQ burgesQ force-pushed the master-make-3 branch 3 times, most recently from 3dec7e6 to 628754c Compare July 3, 2025 14:11
burgesQ added 2 commits July 3, 2025 16:12
One may enable the shared cache by setting the following prior to
loading the fast-synthax-highlighting module

```sh
typeset -gA FAST_HIGHLIGHT
FAST_HIGHLIGHT[chroma-make-cache-global]=1
```

This new implementation change the way the cached makefile' parsed
content are expired. Prior to this commit, cached item expired after
7s. The new implementation get ride of the timer limit and
replace it with a good old "is my file content's different"
@phush0
Copy link

phush0 commented Jul 3, 2025

works as expected here

@burgesQ burgesQ changed the title fix(chroma/make): huge delay on big units feat(chroma/make): new makefile colorization caching strategy Jul 4, 2025
@pschmitt pschmitt merged commit 5ecd353 into zdharma-continuum:master Jul 16, 2025
1 check passed
@HearseDev
Copy link

HearseDev commented Jul 16, 2025

@pschmitt can this not be the default, because this literally makes the shell unusable on large makefile units.

@HearseDev
Copy link

HearseDev commented Jul 16, 2025

@burgesQ @phush0 @pschmitt how do I disable this in upstream or go back to the original behavior without modifying the repo, because it literally freezes my terminal. How is this in the main branch with huge delays/freezes, are there no tests performed with large makefile units, or makefile build systems before it's merged.

I have tried both:

atinit"ZINIT[COMPINIT_OPTS]=-C; zicompinit; _load_local_completions;" \
atload"FAST_HIGHLIGHT[chroma-make-cache-global]=0;" \
    zdharma-continuum/fast-syntax-highlighting \
...

and
FAST_HIGHLIGHT[chroma-make-cache-global]=1;

@HearseDev
Copy link

HearseDev commented Jul 16, 2025

both global and 7sec delay strategies cause the terminal to freeze on large makefile units/build systems with multiple makefiles, it would be the logical thing to regress these changes and go back to old behavior until this can be reproduced/tested and fixed.

@phush0
Copy link

phush0 commented Jul 16, 2025

This is what I tried to achive but no one took it, design is broken because make is called always nomatter the cache

@burgesQ
Copy link
Contributor Author

burgesQ commented Jul 16, 2025

#83 skip the call to make if cache sharing isn't enabled - should fix ur cases sirs @phush0 @HearseDev

Btw if one could share a buggy Makefile so I could run tests against it.

@burgesQ burgesQ deleted the master-make-3 branch July 16, 2025 10:30
@HearseDev
Copy link

#83 skip the call to make if cache sharing isn't enabled - should fix ur cases sirs @phush0 @HearseDev

Btw if one could share a buggy Makefile so I could run tests against it.

https://github.com/theos/theos and use nic.pl to generate any project, and just type make

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants